proxmox_zone - new module #176

JanaHoch · 2025-08-30T06:05:37Z

SUMMARY

New modules to list/creat/update/delete SDN zones

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

proxmox_zone
proxmox_zone_info

ADDITIONAL INFORMATION

This is part of #30

- List zones

- Move sdn locking functions to proxmox module_utils

Thulium-Drake · 2025-09-03T13:36:57Z

Thanks for this work! :-) Can you have a look at the sanity checks (for this and the other PRs you've created)

- Fix PEP8 issues - Fix Doc issues for dnszone - Add workaround for author string - Add attribute check_mode and diff_mode - Add no_log=False for lock_token as this is not a secret - Add proxmox_zone to runtime.yml

JanaHoch · 2025-09-05T19:41:05Z

Thanks for this work! :-) Can you have a look at the sanity checks (for this and the other PRs you've created)

I've fixed sanity issues in all PRs please check. let me know if anything else is needed. Thanks.

JanaHoch · 2025-09-06T09:10:19Z

sorry 1 more question I was trying merge on my local repo and it is giving merge conflicts for meta/runtime.yml do we need to add all new modules to that or if that is not required I can remove it so you won't get those conflicts.

because sanity check is giving this without that meta/runtime.yml: module 'proxmox_zone' is not part of 'proxmox' action group but it's in white so not sure if it's really needed or not.

Thulium-Drake · 2025-09-08T09:49:36Z

I'm not too familiar why, but what I know is that each PR that adds a new module should add it to that list in the runtime.yml. But if you're trying to merge it locally, you should do the following to avoid merge conflicts:

Merge a branch into your main branch
Checkout the second branch you want to merge
Rebase it on the latest state of main
Repeat

The reason you get merge conflicts is because in each branch you add one line after the line that is in the current main. Which, after merging one of those branches, has changed into something else. So from git's perspective your branches want to change the same line.

codecov · 2025-09-08T09:49:45Z

Codecov Report

❌ Patch coverage is 83.87097% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.57%. Comparing base (0d15777) to head (5fef669).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
plugins/modules/proxmox_zone.py	80.00%	13 Missing and 6 partials ⚠️
plugins/module_utils/proxmox_sdn.py	54.05%	17 Missing ⚠️
...sts/unit/plugins/modules/test_proxmox_zone_info.py	88.63%	4 Missing and 1 partial ⚠️
tests/unit/plugins/modules/test_proxmox_zone.py	95.89%	2 Missing and 1 partial ⚠️
plugins/modules/proxmox_zone_info.py	96.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #176      +/-   ##
==========================================
+ Coverage   60.29%   63.57%   +3.28%     
==========================================
  Files          53       60       +7     
  Lines        5641     6315     +674     
  Branches     1127     1235     +108     
==========================================
+ Hits         3401     4015     +614     
- Misses       2089     2128      +39     
- Partials      151      172      +21

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

JanaHoch · 2025-09-08T15:10:40Z

I'm not too familiar why, but what I know is that each PR that adds a new module should add it to that list in the runtime.yml. But if you're trying to merge it locally, you should do the following to avoid merge conflicts:
* Merge a branch into your `main` branch

* Checkout the second branch you want to merge

* Rebase it on the latest state of `main`

* Repeat
The reason you get merge conflicts is because in each branch you add one line after the line that is in the current main. Which, after merging one of those branches, has changed into something else. So from git's perspective your branches want to change the same line.

Ok. let me add it. and also let me check what I can do for unit tests.

JanaHoch · 2025-09-10T16:23:12Z

Hi this is also ready. once this gets merged I'll rebase my vnet and subnet branches so they'll also get locking function for testing. Thanks.

Thulium-Drake · 2025-09-11T10:02:20Z

Let's focus on this and #183 first then. I'm not in a hurry to get a release out. I do think after your PRs have been merged we have a nice juicy release! ;-)

Thulium-Drake · 2025-09-11T12:40:22Z

@IamLunchbox Can you take a look at this again, if you're happy, I'm happy.

I don't use SDN/PVE firewalls (nor understand how they work 😅 ).

@JanaHoch Thanks for making the unit tests, they seem to pass just fine and look extensive!

JanaHoch · 2025-09-11T19:30:56Z

Hi @Thulium-Drake / @IamLunchbox , Sorry I just saw that I forgot to convert few ansible boolean to proxmox bool. I've fixed it now with 7b24750 . Let me know if you see anything else I've missed. I used ansible_to_proxmox_bool() so it should still return None if values are not passed and it shouldn't cause any issue for zone type where those aren't supposed to be present.

IamLunchbox · 2025-09-12T07:12:23Z

Just FYI:
I will need some time to review the PR's, at least until sunday.

JanaHoch · 2025-09-12T17:47:08Z

Just FYI: I will need some time to review the PR's, at least until sunday.

No worries. take your time. I'll also try to finish writing unit tests for remaining PRs. Thanks.

IamLunchbox · 2025-09-13T11:53:05Z

plugins/module_utils/proxmox.py

+            return self.proxmox_api.cluster().sdn().lock().post()
+        except Exception as e:
+            self.module.fail_json(
+                msg=f'Failed to acquire global sdn lock {e}'


@Thulium-Drake The last time i contributed to community.general f-strings were not allowed due to backwards compatibility. But sanity-tests pass on this code. Do you know why? Has python2 compatibility been deprecated for this collection?

In README python 3.7 is mentioned so I thought it should be ok. If not let me know i can update but python2 deprecated when the only python I knew was a snake... :)

Ah i see, very nice.

So we could use type hints, as well now, @Thulium-Drake ?

Ehm, good question, @felixfontein what are the guidelines in community.general for this?

Whether Python 2 compatibility is still needed depends on a) the supported ansible-core versions of the collection, and b) on the collection itself as well (there might be further restrictions on target version numbers).

For community.general, we usually support all ansible-core versions that are not EOL and haven't been EOL for more than a few weeks at the time of a new major release. That means that community.general 11.0.0 (released in spring) still supports ansible-core 2.16, which still supports Python 2.7 on the target. But the next major release, community.general 12.0.0 (should be released in November), will only support ansible-core >= 2.17, and thus will be Python 3.7+.

(I'm not saying this policy is great, but at least it is predictable and doesn't let the CI matrix explode... like community.crypto 2.x.y, which supports everything from Ansible 2.9 up to ansible-core 2.19, or community.internal_test_tools and community.library_inventory_filtering_v1, which even cover Ansible 2.9 up to ansible-core devel :) )

community.proxmox 1.0.0 said it only supports ansible-core 2.17+ from the beginning, so it was always Python 3.7+.

plugins/modules/proxmox_zone.py

IamLunchbox · 2025-09-13T11:58:29Z

plugins/modules/proxmox_zone.py

+EXAMPLES = r"""
+- name: Get all zones
+  community.proxmox.proxmox_zone:
+    api_user: "root@pam"
+    api_password: "{{ vault.proxmox.root_password }}"
+    api_host: "{{ pc.proxmox.api_host }}"
+    validate_certs: no
+
+- name: Get all simple zones
+  community.proxmox.proxmox_zone:
+    api_user: "root@pam"
+    api_password: "{{ vault.proxmox.root_password }}"
+    api_host: "{{ pc.proxmox.api_host }}"
+    validate_certs: no
+    type: simple
+  register: zones


This module tries to retreive information and change SDNs in one go. IMHO, this is counterintuitive and not granular.

Since other modules within ansible core and in this collection are also splitting gathering information and changing things into two different modules, I would suggest doing the same here and creating module zone_info. As an example: The usage between gathering info and changing stuff is split between proxmox_vm_info and proxmox_vm or slurp and copy.

ahh, I did it in this and few other PRs as well because even for update/create/delete I wanted to verify if it exists or not and I'm calling that get method. if I split it in zone_info and zone module is it ok if i duplicate those get methods? or let me see if i can create an object of zone_info in zone and try to access those get methods like that.

Or you could create the information retrieval function in module_utils.

I checked the usage of some other examples in there and there are some, which only have one call through their associated module, so two usages in your case should be fine. I don't know if cross-calling functions from two modules is allowed or not. But I suppose, that module_utils is exactly the place for these kind of use cases-

Ok thanks, I'll split this and other modules and add the get methods in module_utils.

@IamLunchbox / @Thulium-Drake , Do you think it'll be better to create another class in module_utils another base class for all SDN utilities. I can add get methods from all other PRs there and all these locking functions. so that It'll be bit more organized? if this sounds stupid let me know I can always keep it in original base class. And of course this new class's parent will be ProxmoxAnsible

this PR should be ready now. I've created new SDN base class. and fixed all the other things you mentioned and marked them resolved. except for few which need confirmation. if there is still any change needed let me know.

plugins/modules/proxmox_zone.py

IamLunchbox · 2025-09-13T12:08:57Z

plugins/modules/proxmox_zone.py

+  - community.proxmox.attributes
+"""
+
+EXAMPLES = r"""


@Thulium-Drake Should example tasks be added to the intergration tests only after #7 is solved?

Yes, we're still working on it (progress is a bit slow unfortunately), but my plan is that after we get the server up, we'll start working on a molecule setup that will validate some basic modules first.

And then expand it where we can :-)

IamLunchbox · 2025-09-13T12:10:57Z

Meh, sorry for the spam! I should've done the reviews in one review-context I guess?

Anyways, I am about 50% done now, mostly the documentation. Please remember to prepare a changelog fragment.

JanaHoch · 2025-09-13T17:05:08Z

Meh, sorry for the spam! I should've done the reviews in one review-context I guess?

Anyways, I am about 50% done now, mostly the documentation. Please remember to prepare a changelog fragment.

Don't worry about spam. It's okay.
for changelog I thought fragment wasn't needed for new module. Do i need to do it for proxmox module_utils one?

IamLunchbox · 2025-09-13T23:07:16Z

for changelog I thought fragment wasn't needed for new module. Do i need to do it for proxmox module_utils one?

You are right, the new module should be picked up automatically. Regarding the update of module utils: I, personally, think, that a fragment for these new functions is not needed. They are focused on sdn usage and won't impact other modules. @Thulium-Drake , what do you think?

IamLunchbox

I finished the rest of the file review. I did only cursory read the unit test, since it will probably get changed.

IamLunchbox · 2025-09-13T23:19:38Z

plugins/modules/proxmox_zone.py

+EXAMPLES = r"""
+- name: Get all zones
+  community.proxmox.proxmox_zone:
+    api_user: "root@pam"
+    api_password: "{{ vault.proxmox.root_password }}"
+    api_host: "{{ pc.proxmox.api_host }}"
+    validate_certs: no
+
+- name: Get all simple zones
+  community.proxmox.proxmox_zone:
+    api_user: "root@pam"
+    api_password: "{{ vault.proxmox.root_password }}"
+    api_host: "{{ pc.proxmox.api_host }}"
+    validate_certs: no
+    type: simple
+  register: zones


Or you could create the information retrieval function in module_utils.

I checked the usage of some other examples in there and there are some, which only have one call through their associated module, so two usages in your case should be fine. I don't know if cross-calling functions from two modules is allowed or not. But I suppose, that module_utils is exactly the place for these kind of use cases-

plugins/modules/proxmox_zone.py

plugins/module_utils/proxmox.py

plugins/modules/proxmox_zone.py

- Move SDN locking methods from ProxmoxAnsible class to ProxmoxSdnAnsible - Add ge_zones() method to ProxmoxSdnAnsible As we will be splitting zone module into zone and zone_info

- Split proxmox_zone and create proxmox_zone_info - Rename variable type to zone_type

- also fix minor issues in proxmox_zone,proxmox_zone_info, proxmox_sdn.

Thulium-Drake · 2025-09-17T13:35:07Z

for changelog I thought fragment wasn't needed for new module. Do i need to do it for proxmox module_utils one?

You are right, the new module should be picked up automatically. Regarding the update of module utils: I, personally, think, that a fragment for these new functions is not needed. They are focused on sdn usage and won't impact other modules. @Thulium-Drake , what do you think?

Correct, new modules don't require changelogs as the changelog tool will pick them up itself

Thulium-Drake · 2025-09-17T13:39:29Z

Thanks @JanaHoch and @IamLunchbox for your work! I'm sorry I can't help you more with the intricate details of the python programming. I'm more of a Ansible poweruser and not so much a Python programmer 😅, but I'm happy to help where I can!

With regards to the module, the tests pass and you both seem happy with the results ;-)

IamLunchbox · 2025-09-17T13:58:05Z

Hm, I wouldve liked to review the fixed result - I did not know if the changes are all implemented. But what done is done i guess :|

Thulium-Drake · 2025-09-17T14:39:34Z

Well, it's not released yet, I could still revert it if you want. Please let me know how you would like to proceed :-)

felixfontein · 2025-09-17T16:10:33Z

You are right, the new module should be picked up automatically. Regarding the update of module utils: I, personally, think, that a fragment for these new functions is not needed. They are focused on sdn usage and won't impact other modules. @Thulium-Drake , what do you think?

Correct, new modules don't require changelogs as the changelog tool will pick them up itself

In case anyone wonders on how that works: it looks at the top-level version_added. As long as that's there and has the value of the next minor release, the module will be picked up. I just took a quick look, this PR doesn't have version_added entries, so the mechanism won't work. So something for a follow-up :)

IamLunchbox · 2025-09-17T22:40:56Z

Thank you for the heads up felix :)

I finished reviewing and created #189 to fix the missing release tag.

JanaHoch · 2025-09-20T06:40:34Z

Thank you all

JanaHoch added 10 commits August 28, 2025 09:55

proxmox_zone: new module for proxmox_zones

d76ddee

- List zones

proxmox_zone: Create new zone

d8ac153

proxmox_zone: Keep common parameter for all conditions

dee432f

proxmox_zone: Implement locking

2e27f61

proxmox_zone: Added update_zone()

fa60bb4

proxmox_zone: added zone_absent()

de14226

proxmox_zone: Added document and examples

57c7e7d

proxmox_zone: add missing exception handling for zone_preent()

3f82ab6

proxmox_zone: validate params

70d3d83

proxmox_zone and proxmox module_utils

0c0bb74

- Move sdn locking functions to proxmox module_utils

proxmox_zone: Fix sanity issues

a5773bf

- Fix PEP8 issues - Fix Doc issues for dnszone - Add workaround for author string - Add attribute check_mode and diff_mode - Add no_log=False for lock_token as this is not a secret - Add proxmox_zone to runtime.yml

JanaHoch force-pushed the feature/zones branch from 1172fe3 to a5773bf Compare September 5, 2025 19:22

This was referenced Sep 6, 2025

new_module proxmox_subnet #181

Open

new module proxmox_vnet #182

Open

JanaHoch marked this pull request as draft September 10, 2025 05:14

JanaHoch added 2 commits September 10, 2025 21:48

proxmox_zone: fix minor issues found during testing

e69c8d9

proxmox_zone: Add unit tests

8e549c3

JanaHoch marked this pull request as ready for review September 10, 2025 16:21

proxmox_zone: Add missing boolean conversion

7b24750

IamLunchbox reviewed Sep 13, 2025

View reviewed changes

plugins/modules/proxmox_zone.py Outdated Show resolved Hide resolved

IamLunchbox reviewed Sep 13, 2025

View reviewed changes

plugins/modules/proxmox_zone.py Show resolved Hide resolved

IamLunchbox reviewed Sep 13, 2025

View reviewed changes

plugins/modules/proxmox_zone.py Outdated Show resolved Hide resolved

IamLunchbox reviewed Sep 13, 2025

View reviewed changes

IamLunchbox suggested changes Sep 14, 2025

View reviewed changes

JanaHoch added 3 commits September 14, 2025 14:21

module_utils: Create new base class ProxmoxSdnAnsible()

4693b66

- Move SDN locking methods from ProxmoxAnsible class to ProxmoxSdnAnsible - Add ge_zones() method to ProxmoxSdnAnsible As we will be splitting zone module into zone and zone_info

Create seprate proxmox_zone_info

cf1f4c2

- Split proxmox_zone and create proxmox_zone_info - Rename variable type to zone_type

proxmox_zone: Merge statepresent and update

5741796

JanaHoch force-pushed the feature/zones branch from b63366e to f9e0ccb Compare September 14, 2025 14:56

JanaHoch added 2 commits September 15, 2025 08:09

proxmox_zone: Get lock just before making changes

0cf6a3b

- also fix minor issues in proxmox_zone,proxmox_zone_info, proxmox_sdn.

Update unit tests for proxmox_zone & proxmox_zone_info

5fef669

JanaHoch force-pushed the feature/zones branch from f9e0ccb to 5fef669 Compare September 15, 2025 02:41

Thulium-Drake merged commit ca053b8 into ansible-collections:main Sep 17, 2025
15 checks passed

proxmox_zone - new module #176

proxmox_zone - new module #176

Uh oh!

Conversation

JanaHoch commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Uh oh!

Thulium-Drake commented Sep 3, 2025

Uh oh!

JanaHoch commented Sep 5, 2025

Uh oh!

JanaHoch commented Sep 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Thulium-Drake commented Sep 8, 2025

Uh oh!

codecov bot commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JanaHoch commented Sep 8, 2025

Uh oh!

JanaHoch commented Sep 10, 2025

Uh oh!

Thulium-Drake commented Sep 11, 2025

Uh oh!

Thulium-Drake commented Sep 11, 2025

Uh oh!

JanaHoch commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IamLunchbox commented Sep 12, 2025

Uh oh!

JanaHoch commented Sep 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JanaHoch Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JanaHoch Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IamLunchbox commented Sep 13, 2025

Uh oh!

JanaHoch commented Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IamLunchbox commented Sep 13, 2025

Uh oh!

IamLunchbox left a comment

JanaHoch commented Aug 30, 2025 •

edited

Loading

JanaHoch commented Sep 6, 2025 •

edited

Loading

codecov bot commented Sep 8, 2025 •

edited

Loading

JanaHoch commented Sep 11, 2025 •

edited

Loading

JanaHoch Sep 13, 2025 •

edited

Loading

JanaHoch Sep 14, 2025 •

edited

Loading

JanaHoch commented Sep 13, 2025 •

edited

Loading